Skip to content

Conversation

@pquentin
Copy link
Member

@pquentin pquentin commented Oct 3, 2025

Should fix the following test:

// Test file: /test/free/aggregations/range.yml
// Test name: Range aggregation on date field

import { expectAssignable } from 'tsd'
import * as T from '../types'

expectAssignable<T.SearchRequest>({
  "body": {
    "aggs": {
      "date_range": {
        "range": {
          "field": "date",
          "ranges": [
            {
              "from": "2021-05-01T00:00:00Z",
              "to": "2021-05-05T00:00:00Z"
            }
          ]
        }
      }
    },
    "size": 0
  },
  "index": "date_range_test",
  "typed_keys": true
})

@pquentin pquentin added the skip-backport This pull request should not be backported label Oct 3, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2025

Following you can find the validation changes against the target branch for the API.

API Status Request Response
search 🔴 2651/2669 → 2652/2669 2669/2669

You can validate this API yourself by using the make validate target.

@flobernd
Copy link
Member

Let me check the consequences of this change for the .NET client please, before you merge it.

FieldValue does not look like the best candidate here at a first glance. Would DateTime work? If I remember correctly this type alles string and timestamp representation. However, not sure why the range was a double before. AFAIK we do have double timestamp types as well, but not yet a combined version like DateTime.

We should probably introduce a new type of DateTime does not fit, to allow statically typed languages to use their native DateTime primitives like we do in all other cases that deal with date/time values.

@flobernd
Copy link
Member

Ok, so I checked this in detail and I think the test is simply not valid.

"aggs": {
  "date_range": {
    "range": {

This part makes no sense. date_range does not support a range field. It supports ranges which is correctly typed as ranges?: DateRangeExpression[] and therefore supports both representations (double and string).

The AggregationRange type you changed in this PR is used by GeoDistanceAggregation and the regular RangeAggregation but does not affect DateRangeAggregation.

WDYT @l-trotta @Anaethelion ?

@l-trotta
Copy link
Contributor

@flobernd this is not a date_range aggregation... it's a range aggregation, named "date_range" ^^" still, I also think this test is wrong, I can find no evidence server side that from and to can be strings in the context of range aggregation

@flobernd
Copy link
Member

Ah right, the first key is the name of the aggregation. Confusing 😵‍💫

@l-trotta
Copy link
Contributor

had to stare at it for a good 2 minutes

@flobernd
Copy link
Member

flobernd commented Nov 7, 2025

@pquentin @l-trotta

I can find no evidence server side that from and to can be strings in the context of range aggregation

Should we remove the faulty test instead?

@pquentin
Copy link
Member Author

pquentin commented Nov 7, 2025

This was fixed intentionally in elastic/elasticsearch#82732

I can skip the test, but Elasticsearch does accept this query:

{
  "api": "search",
  "file": "/test/free/aggregations/range.yml",
  "name": "Range aggregation on date field",
  "origin": "yaml",
  "request": {
    "args": {
      "body": {
        "aggs": {
          "date_range": {
            "range": {
              "field": "date",
              "ranges": [
                {
                  "from": "2021-05-01T00:00:00Z",
                  "to": "2021-05-05T00:00:00Z"
                }
              ]
            }
          }
        },
        "size": 0
      },
      "index": "date_range_test",
      "typed_keys": true
    }
  },
  "response": {
    "headers": {
      "content-type": "application/json",
      "transfer-encoding": "chunked",
      "x-elastic-product": "Elasticsearch"
    },
    "payload": {
      "_shards": {
        "failed": 0,
        "skipped": 0,
        "successful": 1,
        "total": 1
      },
      "aggregations": {
        "range#date_range": {
          "buckets": [
            {
              "doc_count": 4,
              "from": 1619827200000,
              "from_as_string": "2021-05-01T00:00:00.000Z",
              "key": "2021-05-01T00:00:00.000Z-2021-05-05T00:00:00.000Z",
              "to": 1620172800000,
              "to_as_string": "2021-05-05T00:00:00.000Z"
            }
          ]
        }
      },
      "hits": {
        "hits": [
        ],
        "max_score": null,
        "total": {
          "relation": "eq",
          "value": 5
        }
      },
      "timed_out": false,
      "took": 1
    },
    "statusCode": 200
  }
}

@flobernd
Copy link
Member

flobernd commented Nov 8, 2025

Let's discuss this in the next specification meeting @pquentin 🙂

I think to make this perfect, we have so use untyped variant like for RangeQuery.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-backport This pull request should not be backported specification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants